fix: address Copilot review comments from PR #22 by mars167 · Pull Request #23 · mars167/git-ai-cli

mars167 · 2026-02-11T17:46:54Z

Summary

Fixes all 10 Copilot review comments from PR #22 (query-files command).

Changes

Security

Path traversal protection: resolveWikiDir now rejects paths that escape repoRoot (e.g. --wiki ../../...)

Bug Fixes

Wildcard SQL prefilter: Added globToSqlLike() to properly convert glob patterns (*→%, ?→_) for SQL LIKE queries. Previously src/*/handlers would never match.
Workspace mode: Return explicit error for query-files in workspace mode since queryManifestWorkspace queries by symbol, not file name
Commander type conversion: Numeric options (limit, maxCandidates, repoMapFiles, repoMapSymbols) are now properly parsed as integers before passing to handler

Code Quality

Extract shared helpers: Moved duplicated utilities (isCLIError, buildRepoMapAttachment, resolveWikiDir, inferLangFromFile, filterWorkspaceRowsByLang) into sharedHelpers.ts, used by both queryHandlers.ts and queryFilesHandlers.ts
Remove unused import: Removed resolveLangs from queryFilesHandlers.ts

Test Improvements

Wildcard test: Added assertions for returned rows count and pattern matching
Language filtering test: Added assertion verifying returned files match requested lang
Empty pattern test: Now validates via SearchFilesSchema.parse() throws (Zod validation)
Invalid mode test: Now validates via SearchFilesSchema.parse() throws (Zod validation)
Unused variable: Removed unused result in empty pattern test

Closes review comments from #22

- Extract shared helpers (isCLIError, buildRepoMapAttachment, resolveWikiDir, inferLangFromFile, filterWorkspaceRowsByLang) into sharedHelpers.ts to eliminate duplication between queryHandlers.ts and queryFilesHandlers.ts - Add path traversal protection in resolveWikiDir: reject paths that escape repoRoot - Fix wildcard SQL prefilter: convert glob patterns to SQL LIKE patterns (globToSqlLike) instead of treating raw glob as substring - Return explicit error for workspace mode in query-files since queryManifestWorkspace queries by symbol, not file name - Remove unused import (resolveLangs) from queryFilesHandlers.ts - Convert Commander.js string options to numbers in queryFilesCommand.ts (limit, maxCandidates, repoMapFiles, repoMapSymbols) - Improve tests: add proper assertions for wildcard, language filtering, empty pattern (via Zod schema), and invalid mode (via Zod schema) - Remove unused variable in empty pattern test

chatgpt-codex-connector · 2026-02-11T17:47:00Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

mars167

Review completed by CodaGraph AI Agent.

Additional Comments

The following comments could not be placed inline:

src/cli/handlers/queryHandlers.ts (Line L10-L14)

⚠️ WARNING: 导入依赖的 sharedHelpers 文件需验证存在性

代码将 isCLIError、buildRepoMapAttachment、filterWorkspaceRowsByLang 三个函数从当前文件删除，并改为从 './sharedHelpers' 导入。但 diff 未显示 sharedHelpers 文件的变更，需确认该文件存在且正确导出这些函数，否则会导致模块加载失败

建议: 确认 ./sharedHelpers.ts 文件已创建并正确导出三个函数：export { isCLIError, buildRepoMapAttachment, filterWorkspaceRowsByLang }

// sharedHelpers.ts 应包含:
export { isCLIError } from './sharedHelpers';
export { buildRepoMapAttachment } from './sharedHelpers';
export { filterWorkspaceRowsByLang } from './sharedHelpers';
// 或
import { isCLIError, buildRepoMapAttachment, filterWorkspaceRowsByLang } from './sharedHelpers';

src/cli/handlers/queryHandlers.ts (Line L2-L6, DEL)

💡 SUGGESTION: 被移除的导入语句可能残留未清理

删除了 import { checkIndex, resolveLangs } from '../../core/indexCheck' 和 import { generateRepoMap, type FileRank } from '../../core/repoMap'，需确认这些导入在其他文件中不再需要，避免遗留死代码

建议: 全局搜索 checkIndex、resolveLangs、generateRepoMap、FileRank 确保无其他引用

mars167 · 2026-02-11T17:49:26Z

src/cli/commands/queryFilesCommand.ts

  .option('--wiki <dir>', 'Wiki directory (default: docs/wiki or wiki)', '')
  .action(async (pattern, options) => {
-    await executeHandler('query-files', { pattern, ...options });
+    const limit = parseInt(options.limit, 10);


⚠️ WARNING: parseInt 失败时未处理 NaN

parseInt 在处理非数字字符串时返回 NaN（如 --limit abc），后续 executeHandler 收到 NaN 可能导致静默错误或意外行为

建议: 添加验证逻辑，过滤无效值：
const limit = /^-?\d+$/.test(options.limit) ? parseInt(options.limit, 10) : undefined;
或使用默认值兜底

Suggested change

const limit = parseInt(options.limit, 10);

const limit = parseInt(options.limit, 10);

if (isNaN(limit)) {

console.error(`Invalid limit: ${options.limit}`);

return;

}

mars167 · 2026-02-11T17:49:27Z

src/cli/commands/queryFilesCommand.ts

  .option('--wiki <dir>', 'Wiki directory (default: docs/wiki or wiki)', '')
  .action(async (pattern, options) => {
-    await executeHandler('query-files', { pattern, ...options });
+    const limit = parseInt(options.limit, 10);


💡 SUGGESTION: 缺乏参数验证提示

CLI 参数错误时未向用户输出友好的错误信息，用户无法得知应如何修正

建议: 捕获无效输入并输出用法提示，帮助用户正确使用命令

mars167 · 2026-02-11T17:49:27Z

src/cli/handlers/sharedHelpers.ts

+      wikiDir,
+    });
+    return { enabled: true, wikiDir, files };
+  } catch (e: any) {


⚠️ WARNING: 使用 any 丢失错误类型信息

catch 块中使用 e: any 掩盖了错误的类型信息，不利于调试和错误追踪

建议: 使用 unknown 替代 any，并通过类型守卫安全访问错误信息

Suggested change

} catch (e: any) {

} catch (e: unknown) {

const message = e instanceof Error ? e.message : String(e);

return { enabled: false, skippedReason: message };

}

mars167 · 2026-02-11T17:49:27Z

src/cli/handlers/sharedHelpers.ts

+  if (w) {
+    const resolved = path.resolve(repoRoot, w);
+    // Prevent path traversal outside repoRoot
+    if (!resolved.startsWith(repoRoot + path.sep) && resolved !== repoRoot) {


⚠️ WARNING: 路径遍历检查存在跨平台兼容性问题

使用 path.sep 进行路径比较在 Windows (\) 和 Unix (/) 混合环境中可能失效。推荐使用 path.relative() 进行更健壮的路径校验

建议: 改用 path.relative 进行安全校验

Suggested change

if (!resolved.startsWith(repoRoot + path.sep) && resolved !== repoRoot) {

const relative = path.relative(repoRoot, resolved);

if (relative.startsWith('..') || path.isAbsolute(relative)) {

return '';

}

mars167 · 2026-02-11T17:49:27Z

src/cli/handlers/sharedHelpers.ts

+  return 'ts';
+}
+
+export function filterWorkspaceRowsByLang(rows: any[], langSel: string): any[] {


💡 SUGGESTION: 函数参数和返回值缺少类型定义

函数签名使用 any[] 类型，丢失了 TypeScript 的类型安全优势

建议: 定义明确的接口类型来描述 rows 的结构

Suggested change

export function filterWorkspaceRowsByLang(rows: any[], langSel: string): any[] {

interface WorkspaceRow {

file: string;

// 其他字段...

}

export function filterWorkspaceRowsByLang(rows: WorkspaceRow[], langSel: string): WorkspaceRow[] {

mars167 · 2026-02-11T17:49:27Z

src/cli/handlers/sharedHelpers.ts

+import type { CLIError } from '../types';
+
+export function isCLIError(value: unknown): value is CLIError {
+  return typeof value === 'object' && value !== null && 'ok' in value && (value as any).ok === false;


📝 NIT: 类型守卫可进一步优化

使用 (value as any).ok 绕过了类型检查，略显冗余

建议: 使用可选链和更严格的类型检查

Suggested change

return typeof value === 'object' && value !== null && 'ok' in value && (value as any).ok === false;

return (

typeof value === 'object' &&

value !== null &&

'ok' in value &&

(value as CLIError).ok === false

);

Copilot

Pull request overview

This PR addresses prior review feedback for the query-files CLI command by hardening wiki path handling, fixing wildcard SQL prefiltering, clarifying unsupported workspace behavior, and reducing duplicated handler utilities.

Changes:

Adds shared handler utilities (sharedHelpers.ts) and updates query/query-files handlers to use them.
Fixes wildcard SQL prefiltering by converting glob patterns to SQL LIKE patterns.
Improves query-files behavior/tests (schema-level validation tests, stronger wildcard/lang assertions) and parses numeric CLI options before handler execution.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/queryFiles.test.ts	Strengthens assertions and shifts some invalid-input checks to Zod schema validation.
src/cli/handlers/sharedHelpers.ts	Introduces shared helpers incl. `resolveWikiDir` containment check and repo-map attachment builder.
src/cli/handlers/queryHandlers.ts	Removes duplicated helpers and uses shared helper implementations.
src/cli/handlers/queryFilesHandlers.ts	Adds glob→SQL LIKE conversion and changes workspace mode to an explicit error path.
src/cli/commands/queryFilesCommand.ts	Parses numeric CLI options prior to calling `executeHandler`.
package-lock.json	Version bump / lockfile metadata updates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-11T18:09:55Z

src/cli/handlers/sharedHelpers.ts

+/**
+ * Resolve wiki directory, ensuring the resolved path stays within repoRoot
+ * to prevent path traversal attacks.
+ */
+export function resolveWikiDir(repoRoot: string, wikiOpt: string): string {


The PR description says wiki path traversal is addressed, but only query/query-files use the new sharedHelpers.resolveWikiDir. repo-map and semantic handlers still have their own resolveWikiDir implementations without the containment check (see src/cli/handlers/repoMapHandler.ts and src/cli/handlers/semanticHandlers.ts). Consider switching those handlers to import/use resolveWikiDir from sharedHelpers as well so the protection is consistent across commands.

Copilot · 2026-02-11T18:09:56Z

src/cli/commands/queryFilesCommand.ts

+    const limit = parseInt(options.limit, 10);
+    const maxCandidates = parseInt(options.maxCandidates, 10);
+    const repoMapFiles = parseInt(options.repoMapFiles, 10);
+    const repoMapSymbols = parseInt(options.repoMapSymbols, 10);
+    await executeHandler('query-files', {
+      pattern,
+      ...options,
+      limit,
+      maxCandidates,
+      repoMapFiles,
+      repoMapSymbols,


Parsing numeric options with parseInt here weakens validation compared to the Zod schema coercion: values like --limit 10abc will be accepted as 10 instead of rejected. Either pass the raw string values through to executeHandler and let SearchFilesSchema coerce/validate them, or explicitly validate that the entire string is a base-10 integer (and error on NaN / trailing junk).

Suggested change

const limit = parseInt(options.limit, 10);

const maxCandidates = parseInt(options.maxCandidates, 10);

const repoMapFiles = parseInt(options.repoMapFiles, 10);

const repoMapSymbols = parseInt(options.repoMapSymbols, 10);

await executeHandler('query-files', {

pattern,

...options,

limit,

maxCandidates,

repoMapFiles,

repoMapSymbols,

await executeHandler('query-files', {

pattern,

...options,

Copilot · 2026-02-11T18:09:56Z

src/cli/handlers/queryFilesHandlers.ts

+import {
+  isCLIError,
+  buildRepoMapAttachment,
+  filterWorkspaceRowsByLang,


filterWorkspaceRowsByLang is imported but no longer used after workspace mode was changed to return an error. Please remove the unused import to keep the module tidy.

Suggested change

filterWorkspaceRowsByLang,

Copilot · 2026-02-11T18:09:56Z

src/cli/handlers/sharedHelpers.ts

+ * Resolve wiki directory, ensuring the resolved path stays within repoRoot
+ * to prevent path traversal attacks.
+ */
+export function resolveWikiDir(repoRoot: string, wikiOpt: string): string {
+  const w = String(wikiOpt ?? '').trim();
+  if (w) {
+    const resolved = path.resolve(repoRoot, w);
+    // Prevent path traversal outside repoRoot
+    if (!resolved.startsWith(repoRoot + path.sep) && resolved !== repoRoot) {
+      return '';
+    }
+    return resolved;
+  }
+  const candidates = [path.join(repoRoot, 'docs', 'wiki'), path.join(repoRoot, 'wiki')];
+  for (const c of candidates) {
+    if (fs.existsSync(c)) return c;
+  }


resolveWikiDir only checks the lexical path prefix via startsWith, which can be bypassed by symlinks inside the repoRoot (e.g., --wiki docs/wikiLink pointing outside). Since generateRepoMap will read files from wikiDir, this can still allow reading outside the repo. Consider comparing fs.realpathSync(repoRoot) vs fs.realpathSync(resolved) (and/or using path.relative on realpaths) and returning an explicit error when the resolved realpath escapes repoRoot rather than silently returning ''.

Suggested change

* Resolve wiki directory, ensuring the resolved path stays within repoRoot

* to prevent path traversal attacks.

*/

export function resolveWikiDir(repoRoot: string, wikiOpt: string): string {

const w = String(wikiOpt ?? '').trim();

if (w) {

const resolved = path.resolve(repoRoot, w);

// Prevent path traversal outside repoRoot

if (!resolved.startsWith(repoRoot + path.sep) && resolved !== repoRoot) {

return '';

}

return resolved;

}

const candidates = [path.join(repoRoot, 'docs', 'wiki'), path.join(repoRoot, 'wiki')];

for (const c of candidates) {

if (fs.existsSync(c)) return c;

}

* Check whether `candidate` is inside (or equal to) `root`, using a relative path

* comparison. Both arguments must be absolute, normalized paths.

*/

function isPathInside(root: string, candidate: string): boolean {

const rel = path.relative(root, candidate);

if (rel === '') return true;

return rel !== '..' && !rel.startsWith('..' + path.sep);

}

/**

* Resolve wiki directory, ensuring the resolved path stays within repoRoot

* (taking symlinks into account) to prevent path traversal attacks.

*/

export function resolveWikiDir(repoRoot: string, wikiOpt: string): string {

const w = String(wikiOpt ?? '').trim();

const rootReal = fs.realpathSync(repoRoot);

if (w) {

const resolved = path.resolve(repoRoot, w);

// If the target exists, validate using realpath to avoid symlink escapes.

if (fs.existsSync(resolved)) {

const resolvedReal = fs.realpathSync(resolved);

if (!isPathInside(rootReal, resolvedReal)) {

throw new Error(`Wiki directory must be inside the repository root: ${resolvedReal}`);

}

return resolvedReal;

}

// For non-existent paths, fall back to lexical containment check against the

// realpath of repoRoot. This still prevents obvious escapes like "../../..".

if (!isPathInside(rootReal, resolved)) {

throw new Error(`Wiki directory must be inside the repository root: ${resolved}`);

}

return resolved;

}

const candidates = [path.join(repoRoot, 'docs', 'wiki'), path.join(repoRoot, 'wiki')];

for (const c of candidates) {

if (!fs.existsSync(c)) continue;

const cReal = fs.realpathSync(c);

if (isPathInside(rootReal, cReal)) {

return cReal;

}

}

mars167 commented Feb 11, 2026

View reviewed changes

mars167 requested a review from Copilot February 11, 2026 18:04

Copilot started reviewing on behalf of mars167 February 11, 2026 18:05 View session

mars167 merged commit 3346bd0 into main Feb 11, 2026
5 checks passed

Copilot AI reviewed Feb 11, 2026

View reviewed changes

-  } catch (e: any) {
+} catch (e: unknown) {
+    const message = e instanceof Error ? e.message : String(e);
+    return { enabled: false, skippedReason: message };
+  }

-export function filterWorkspaceRowsByLang(rows: any[], langSel: string): any[] {
+interface WorkspaceRow {
+  file: string;
+  // 其他字段...
+}
+export function filterWorkspaceRowsByLang(rows: WorkspaceRow[], langSel: string): WorkspaceRow[] {

Conversation

mars167 commented Feb 11, 2026

Summary

Changes

Security

Bug Fixes

Code Quality

Test Improvements

Uh oh!

chatgpt-codex-connector bot commented Feb 11, 2026

Uh oh!

mars167 left a comment

Choose a reason for hiding this comment

Additional Comments

src/cli/handlers/queryHandlers.ts (Line L10-L14)

src/cli/handlers/queryHandlers.ts (Line L2-L6, DEL)

Uh oh!

mars167 Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

mars167 Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

mars167 Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

mars167 Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

mars167 Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

mars167 Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant